[WIP] Authorization#5518
Draft
ramonsmits wants to merge 8 commits into
Draft
Conversation
Add Authorize attribute to all controller APIs
Adds Authentication.RolesClaim (default "realm_access.roles") and a RolesClaimsTransformation that normalises per-IdP role claim shapes — Keycloak's nested realm_access.roles, flat repeated claims (Entra app roles, Keycloak with a User Realm Role mapper, Cognito groups), and JSON-array-as-string values — into canonical "roles" claims that PermissionVerbHandler reads unchanged. The parsing lives in a pure static RolesClaimExtractor in Infrastructure so it can be unit-tested without an ASP.NET host; the IClaimsTransformation implementation in Hosting is a thin wrapper that adds idempotency via a sentinel claim. Removes the TODO on PermissionVerbHandler that the transformation now resolves.
Without RequireAuthenticatedUser() in the policy, an unauthenticated request reaches PermissionVerbHandler, finds no roles, and fails the requirement — which ASP.NET classifies as FailedRequirements and turns into a 403 Forbid. The OIDC acceptance tests expect 401 Challenge (Should_reject_requests_without_bearer_token, ..._with_invalid/expired /wrong_audience/wrong_issuer). Adding the auth requirement first ensures the failure is classified as FailedAuthentication when no valid principal is present.
The Primary, Audit, and Monitoring acceptance-test runners called AddServiceControlAuthentication but not AddServiceControlAuthorization, while production RunCommand wires both. Without the authorization provider, ASP.NET cannot resolve the policy names emitted by the Permissions catalogue and any request to a controller carrying [Authorize(Policy = Permissions.X)] throws "AuthorizationPolicy named '...' was not found" — which breaks the test hosts and times out the audit acceptance tests that exercise authorized endpoints.
Should_accept_requests_with_valid_bearer_token previously passed in CI by accident: AddServiceControlAuthorization was missing from the test runners, so the policy provider could not resolve the [Authorize] policy name and threw, returning 500 — which the assertion "not 401 and not 403" accepted. With the policy provider now wired up, a valid token without any role claim correctly fails the permission requirement and returns 403. The test must therefore supply the canonical "roles" claim with a value that maps to a role granting the endpoint's permission. "reader" grants every *:*:view permission, which covers the endpoints the three acceptance-test variants exercise (error:messages:view, audit:message:view, monitoring:endpoint:view).
The PlatformSampleSettings approval tests in SC, Audit, and Monitoring unit-test projects serialise the settings object and diff it against an approved JSON snapshot. The earlier Authentication.RolesClaim addition needs the snapshots updated to include the new property.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.